Skip to content

[windows] add Python 3.10.1 to the installer #447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

charles-zablit
Copy link

@charles-zablit charles-zablit commented Aug 1, 2025

This patch adds an option to install the embeddable version of Python directly from the toolchain installer on Windows.

The installer will pick up the python files from T:\Program Files\Swift\Python and install them in the bin directory alongside lldb.exe.

This is in continuation of the work in swiftlang/swift#83488.

rdar://157773554

@@ -0,0 +1,5 @@
<Project Sdk="WixToolset.Sdk/4.0.5">
<PropertyGroup>
<OutputName>python</OutputName>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is going be part of the toolchain, we will need to have one per variant. here is my last change for how to make the authoring work for all variants: #444

i can help make that happen if needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need 1 build per variant? I think it should be possible to share the Python installation across the different toolchain variants. We should lay them out such that they are isolated:

[INSTALLROOT]\Swift\Python-3.10.1\...

If we layout python this way, it would be isolated from any system installations and should be possible to share across different variants of the toolchain.

Copy link
Author

@charles-zablit charles-zablit Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INSTALLROOT]\Swift\Python-3.10.1\...

Just to make sure I understand, would the Embeddable Python actually live in [INSTALLROOT]\Swift\Python-3.10.1\...? As in, once installed, python.exe will be at [INSTALLROOT]\Swift\Python-3.10.1\python.exe?
In that case, how would lldb resolve the path to the embeddable python?

My initial plan was to extract the Python files into T:\Program Files\Swift\Python like you suggested here and then install them in the bin directory alongside lldb.exe.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The installation information should be queryable and you can extract that from the registry. This is basically what tcrun does as well.

Version="$(NonSemVerProductVersion)"
Scope="$(PackageScope)">

<Media Id="1" Cabinet="$(VariantCabinetName)" EmbedCab="$(ArePackageCabsEmbedded)" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not going to build without these variables defined. i would recommend moving this file to a wxi, and defining these variables in a wxs file that imports it. see https://github.com/swiftlang/swift-installer-scripts/blob/main/platforms/Windows/bld/asserts/bld.asserts.wxs

Copy link
Author

@charles-zablit charles-zablit Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks! However, I think the file structure in swift-installer-scripts\platforms\Windows\python is not correct: the subdirectory should not be named asserts. Do you have a suggestion of what this should be named? Or should I just adopt a flat directory structure?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that cpy.wxs (as this is CPython) is fine as a name.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, the file structure is:

python/
├─ asserts/
│  ├─ python.wxs
│  ├─ python.wixproject
python.wxi

I will rename the python files to cpy but what should the asserts folder be named?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhegazy regarding the variant name, I would really like to remove asserts for Python as it's not an asserts build, just a regular Python distribution. Would the following file structure be OK?

python/
├─ python.wxs
├─ python.wixproject
├─ python.wxi

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes a lot of sense to me thanks!.

If lldb can look up an env var for LLDB_PYTHON_HOME we can set that during setup time to be the location where we install python

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried to install the embeddable Python alongside the Asserts and NoAsserts directories, with LLDB_PYTHON_HOME=../../Python and tried other CMake defines like LLDB_PYTHON_EXE_RELATIVE_PATH.

This does not work because lldb still tries to look for python310.dll in the Path.

I see 2 possible solutions going forward:

1

Use Windows APIs to manually add specify the location of python310.dll before starting lldb. Since we optionally install Embeddable Python, this adds some hidden logic, if the user uses regular Python instead, in which case the DLL will actually be in the PATH. For this reason, I'm not in favor of this solution.

2

Put the contents of the embeddable Python folder in usr/bin. We would have something like this:

Asserts/
├─ usr/
│  ├─ bin/
│  │  ├─ lldb.exe
│  │  ├─ python.exe
│  │  ├─ python310.dll
NoAsserts/
├─ usr/
│  ├─ bin/
│  │  ├─ lldb.exe
│  │  ├─ python.exe
│  │  ├─ python310.dll

I have tested this scenario and it works fine.

Now your concern here was regarding this scenario:

Asserts/
├─ usr/
│  ├─ bin/
│  │  ├─ lldb.exe
│  │  ├─ python.exe
│  │  ├─ python310.dll
NoAsserts/
├─ usr/
│  ├─ bin/
│  │  ├─ lldb.exe

In this scenario, the Asserts variant works fine. The NoAsserts variant should work fine as well because Asserts/usr/bin will be in the PATH. If it's not, the user will have to install regular Python manually. I think this is OK because they would have had to install it either way. To I don't think that the following is true.

so if we only install to one, the other variant is broken since lldb expects it to be in the path, and it will not be.


I think we should go with the 2nd solution, because it works with minimal changes to lldb's code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think option 2 is a good one. Installing the component per variant is not the best path as we discussed earlier.

Here is the thing, we install usr/bin in the PATH. so if we put python.exe in usr/bin users will find it if they just run python. this is not diffrent from putting it in ../../Python and then adding this to the PATH so that lldb find it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I have another option. lldb needs 2 things, python310.dll since it was linked against it, and cannot load without it, and the rest of the embeded python installation to execute script, and find imports etc..

For the first, we would add a new component under LLDB ComponentGroup for python310.dll - this will be in the PATH since the whole folder is, but it is not an issue since this is the dll and not the python.exe.

For the second, we would install the new python.msi to the ..\..\Python as we discussed before, and set LLDB_PYTHON_HOME=../../Python as part of the installation.

@compnerd what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be problematic as the python.exe (interpreter) is then going to fail to find the DLL. I think that the best option here is to do a LoadLibraryW for python3.10.dll and in the failing case use SetDllDirectory to inject the additional library search path. I just want to verify that it does not change the DLL lookup order if we do that (as in would block the previous search order from being honoured).

</Component>

<Component Directory="toolchain_$(VariantName)_usr_bin">
<File Source="$(PythonRoot)\python.exe" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need the executable? i thought that all lldb needed is the dll and the built in modules?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should include thee executable. Ultimately, the issue with all this is that LLDB is meant to be usable as a scriptable debugger, including for post-mortem scenarios. In such a case, you want to be able to execute a script, which is going to require the interpreter.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need the executable? i thought that all lldb needed is the dll and the built in modules?

On Windows, following this discussion, we want to bundle an executable Python with the toolchain installer.

@@ -9,6 +9,7 @@
Scope="$(PackageScope)">

<?define PlatformRoot = "$(ImageRoot)\Platforms\Windows.platform"?>
<?define PythonRoot = "$(ImageRoot)\python"?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be dedined in python msi project and not here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks!

@@ -19,6 +19,7 @@
<ProjectReference Include="..\bld\asserts\bld.asserts.wixproj" BindName="bld.asserts" />
<ProjectReference Include="..\cli\asserts\cli.asserts.wixproj" BindName="cli.asserts" />
<ProjectReference Include="..\dbg\asserts\dbg.asserts.wixproj" BindName="dbg.asserts" />
<ProjectReference Include="..\python\python.wixproj" BindName="python" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets shuffle this down after all ide MSI. I wonder if it makes sense to rename this to match the 3-letter naming.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it down below ide. Should it be at the very bottom?

Regarding the 3-letter renaming, I'm not opposed to it, however I think it would be good to differentiate between embedded Python and regular Python if we ever decide to bundle/chain the full Python msi with the toolchain installer.

If we are sure we are never going to chain the full Python msi, then we could name this py3?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am certainly curious about why the differentiation matters.

Ultimately, the difference between the embedded and full python is something that I think that we should somewhat blur. The actual difference is the lack of an installer (not a problem), a slightly smaller standard library (is that truly a concern?), and the missing pip which we can inject. So, difference-wise it is pretty small.

They also do not author MSIs, which means that the chaining would be more complicated than if they provided a MSM/MSI.

<Checkbox Name="OptionsInstallAndroidSDKAMD64" X="210" Y="363" Width="-11" Height="17" TabStop="yes" FontId="3" EnableCondition="OptionsInstallAndroidPlatform">#(loc.Sdk_ProductName_Android_amd64)</Checkbox>
<Checkbox Name="OptionsInstallAndroidSDKARM" X="210" Y="381" Width="-11" Height="17" TabStop="yes" FontId="3" EnableCondition="OptionsInstallAndroidPlatform">#(loc.Sdk_ProductName_Android_armv7)</Checkbox>
<Checkbox Name="OptionsInstallAndroidSDKX86" X="210" Y="399" Width="-11" Height="17" TabStop="yes" FontId="3" EnableCondition="OptionsInstallAndroidPlatform">#(loc.Sdk_ProductName_Android_x86)</Checkbox>
<Checkbox Name="OptionsInstallEmbeddedPython" X="192" Y="165" Width="-11" Height="17" TabStop="yes" FontId="3">#(loc.EmbeddedPython_ProductName)</Checkbox>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this should come at the very end as it is not really part of the toolchain, it is a third party dependency we are installing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks 👍

@@ -9,6 +9,7 @@
Scope="$(PackageScope)">

<?define PlatformRoot = "$(ImageRoot)\Platforms\Windows.platform"?>
<?define PythonRoot = "$(ImageRoot)\python"?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@@ -0,0 +1,5 @@
<Project Sdk="WixToolset.Sdk/4.0.5">
<PropertyGroup>
<OutputName>python</OutputName>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need 1 build per variant? I think it should be possible to share the Python installation across the different toolchain variants. We should lay them out such that they are isolated:

[INSTALLROOT]\Swift\Python-3.10.1\...

If we layout python this way, it would be isolated from any system installations and should be possible to share across different variants of the toolchain.

Name="$(VariantProductName)"
UpgradeCode="$(VariantUpgradeCode)"
Version="$(NonSemVerProductVersion)"
Scope="$(PackageScope)">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we version this component? Should we version it as per the Python version or the Swift toolchain version?

</Component>

<Component Directory="toolchain_$(VariantName)_usr_bin">
<File Source="$(PythonRoot)\python.exe" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should include thee executable. Ultimately, the issue with all this is that LLDB is meant to be usable as a scriptable debugger, including for post-mortem scenarios. In such a case, you want to be able to execute a script, which is going to require the interpreter.

@charles-zablit charles-zablit force-pushed the charles-zablit/windows/embed-python-in-the-installer branch from d806397 to 91159fc Compare August 4, 2025 14:38
@charles-zablit charles-zablit changed the title [Draft][windows] add Python 3.10.1 to the installer [windows] add Python 3.10.1 to the installer Aug 4, 2025
<?define PythonRoot = "$(ImageRoot)\Python"?>

<?include ../python.wxi ?>
</Wix>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks 👍


<ComponentGroup Id="EmbeddedPython">
<Component Directory="toolchain_$(VariantName)_usr_bin">
<File Source="$(PythonRoot)\libcrypto-1_1-arm64.dll" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file included on both ARM64 and AMD64?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not, I've added the ArchSuffix define to address this. Thanks!

</Component>

<Component Directory="toolchain_$(VariantName)_usr_bin">
<File Source="$(PythonRoot)\libssl-1_1-arm64.dll" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not, I've added the ArchSuffix define to address this. Thanks!

Comment on lines 78 to 84
<Component Directory="toolchain_$(VariantName)_usr_bin">
<File Source="$(PythonRoot)\vcruntime140.dll" />
</Component>

<Component Directory="toolchain_$(VariantName)_usr_bin">
<File Source="$(PythonRoot)\vcruntime140_1.dll" />
</Component>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This brings up the question - do we want to bundle an additional copy of the VCRuntime? This is an interesting question because the toolchain itself also does depend on the runtime. There is a copy in the system. And now in python. The system one however will get security updates, this will not. Since part of the argument for this upgrade is "security" - we shouldn't be bundling them IMO.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed them 👍

<WixVariable Id="SideBySidePackageUpgradeCode" Value="$(VariantUpgradeCode)" />
<FeatureGroupRef Id="SideBySideUpgradeStrategy" />

<ComponentGroup Id="EmbeddedPython">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the directory attribute be moved to the ComponentGroup instead of the Component? That would simplify the authoring here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks 👍

</Component>
</ComponentGroup>

<Feature Id="EmbeddedPython" AllowAbsent="yes" Title="$(VariantProductName)">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that we should allow this component to be absent. We would install nothing then.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks 👍


<Component Directory="toolchain_$(VariantName)_usr_bin">
<File Source="$(PythonRoot)\LICENSE.txt" />
</Component>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the LICENSE file in /usr/bin? Shouldn't that be under /usr/share/licenses?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not know about usr/share/licenses. Created the directory and added the license there 👍

@charles-zablit charles-zablit self-assigned this Aug 4, 2025
<?define VariantCabinetName = python.asserts.cab?>
<?define ToolchainVersionedVariantDirectory = ToolchainVersionedAsserts ?>
<?define VariantEnvironmentComponentGUID = 30629e0c-b376-47bc-bedf-fefb7d4ca61d?>
<?if $(ProductArchitecture) = "arm64" ?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not variant specific. i would recommend moving it to the wxi to avoid it being duplicated for diffrent variants.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this seems like we should sink it into the wxi.

<?define VariantName = asserts ?>
<?define VariantUpgradeCode = $(PythonUpgradeCode)?>
<?define VariantProductName = !(loc.EmbeddedPython_ProductName)?>
<?define VariantCabinetName = python.asserts.cab?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<?define VariantCabinetName = python.asserts.cab?>
<?define VariantCabinetName = python.cab?>

<?define VariantUpgradeCode = $(PythonUpgradeCode)?>
<?define VariantProductName = !(loc.EmbeddedPython_ProductName)?>
<?define VariantCabinetName = python.asserts.cab?>
<?define ToolchainVersionedVariantDirectory = ToolchainVersionedAsserts ?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the ToolchainVersionedVariantDirectory define now?

@@ -0,0 +1,14 @@
<Wix xmlns="http://wixtoolset.org/schemas/v4/wxs">
<?define VariantName = asserts ?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the VariantName now?

<?define VariantCabinetName = python.asserts.cab?>
<?define ToolchainVersionedVariantDirectory = ToolchainVersionedAsserts ?>
<?define VariantEnvironmentComponentGUID = 30629e0c-b376-47bc-bedf-fefb7d4ca61d?>
<?if $(ProductArchitecture) = "arm64" ?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this seems like we should sink it into the wxi.

<FeatureGroupRef Id="SideBySideUpgradeStrategy" />

<ComponentGroup Id="EmbeddedPython" Directory="toolchain_$(VariantName)_usr_bin">
<Component >
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<Component >
<Component>

</ComponentGroup>

<ComponentGroup Id="EmbeddedPythonLicense">
<Component Directory="toolchain_$(VariantName)_usr_share_licenses">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that we should be adding this to the toolchain layout but rather the python subdirectory.

@@ -55,6 +55,7 @@
<Directory Id="toolchain_asserts_usr_share_docc" Name="docc">
<Directory Id="toolchain_asserts_usr_share_docc_render" Name="render" />
</Directory>
<Directory Id="toolchain_asserts_usr_share_licenses" Name="licenses" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should move into the directory hierarchy for Python.

@@ -165,10 +167,11 @@ msbuild %SourceRoot%\swift-installer-scripts\platforms\Windows\bundle\installer.
-p:Configuration=Release ^
-p:BaseOutputPath=%PackageRoot%\online\ ^
-p:ImageRoot=%ImageRoot%\Program Files\Swift ^
-p:PythonRoot=%ImageRoot%\Program Files\Python-3.10.1 ^
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we want to sink this further 1 level as %ImageRoot%\Program Files\Swift\Python-3.10.1. When we do that, we can remove the parameter as we only need the python version as we know the image root is %ImageRoot%\Program Files\Swift so we can compute the path relative to that as $(ImageRoot)\Python-$(PythonVersion).

-p:Platforms="android;windows" ^
-p:AndroidArchitectures="aarch64;armv7;i686;x86_64" ^
-p:WindowsArchitectures="aarch64;i686;x86_64" ^
-p:WindowsRuntimeARM64=%ImageRoot%\Prograam Files (Arm64)\Swift\Runtimes\0.0.0 ^
-p:WindowsRuntimeARM64=%ImageRoot%\Program Files (Arm64)\Swift\Runtimes\0.0.0 ^
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah! Thanks for fixing my typo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants